Skip to content

Remove redundant data-ampdevmode attributes from admin bar descendants #3701

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Remove redundant data-ampdevmode attributes from admin bar descendants #3701

wants to merge 1 commit into from

Conversation

@westonruter
Copy link
Member

@westonruter westonruter commented Nov 8, 2019

Summary

I found out from @Gregable that data-ampdevmode applies to the entire descendant DOM tree when it is added to an element:

The way it works in the validator is that you add data-ampdevmode to the root <html> tag first. Then, any other tag that includes data-ampdevmode will have any errors suppressed on it or it's descendants.

If you want to do:

<html amp data-ampdevmode>
<head>...</head>
<body data-ampdevmode>
  ... Anything goes in here ...
</body>
</html>

Then that would be consistent with the validator mode. The idea of requiring it twice is that you can decide which parts of the document you want errors for and which parts you don't. You can add it to the body and/or head (*) if you want to just disable all errors.

(*) This doesn't get rid of the mandatory document-level requirement errors, just the tag-level errors. Missing boilerplate will still be reported for example.

This means we can eliminate adding data-ampdevmode to every single element under #wpadminbar in addition to adding it to the #wpadminbar container element as well.

This is a follow-up on #3187.

Checklist

  • My pull request is addressing an open issue (please create one otherwise).
  • My code is tested and passes existing tests.
  • My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

@westonruter westonruter added this to the v1.4.1 milestone Nov 8, 2019
@googlebot googlebot added the cla: yes Signed the Google CLA label Nov 8, 2019
@westonruter westonruter removed the request for review from schlessera November 8, 2019 07:50
@westonruter
Copy link
Member Author

westonruter commented Nov 8, 2019

I just realized that this may not have the fully intended effect, as the function to check if an element is in dev mode does not look at its ancestors.

@westonruter westonruter removed this from the v1.4.1 milestone Nov 8, 2019
@westonruter
Copy link
Member Author

westonruter commented Apr 16, 2020

While this is redundant, it does allow the sanitizers to not have to keep track of whether or not they are inside of a subtree of an element that has the data-ampdevmode attribute.

Maybe later.

@schlessera schlessera deleted the remove/redundant-amp-dev-mode branch November 6, 2020 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

cla: yes Signed the Google CLA

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@westonruter @googlebot